Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: mcli integration #536

Merged
merged 1 commit into from
Aug 27, 2021
Merged

feat: mcli integration #536

merged 1 commit into from
Aug 27, 2021

Conversation

gssbzn
Copy link
Collaborator

@gssbzn gssbzn commented Aug 27, 2021

Description

Add support for MCLI_* env variables for convenience when using both tools

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have read the Terraform contribution guidelines
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

Also includes a couple of small bug fixes when overriding the base url

@gssbzn gssbzn added the run-testacc To run acceptance tests label Aug 27, 2021
PublicKey string
PrivateKey string
BaseURL string
RealmBaseURL string
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realm has a different base url, if I override the one for atlas I need to change the one for realm as well and they should not share it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh makes sense, thank you gustavo

@@ -28,6 +28,8 @@ type MongoDBClient struct {
Config *Config
}

var ua = "terraform-provider-mongodbatlas/" + version.ProviderVersion
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extracting the duplicated UA

@@ -63,12 +65,12 @@ func (c *Config) NewClient(ctx context.Context) (interface{}, diag.Diagnostics)
func (c *MongoDBClient) GetRealmClient(ctx context.Context) (*realm.Client, error) {
// Realm
if c.Config.PublicKey == "" && c.Config.PrivateKey == "" {
return nil, fmt.Errorf("please set `public_key` and `private_key` in order to use the realm client")
return nil, errors.New("please set `public_key` and `private_key` in order to use the realm client")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to format as there's no interpolation

if len(c.Config.BaseURL) > 0 {
optsRealm = append(optsRealm, realm.SetBaseURL(c.Config.BaseURL))
optsRealm := []realm.ClientOpt{realm.SetUserAgent(ua)}
if c.Config.BaseURL != "" && c.Config.RealmBaseURL != "" {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for realm you should override both

Comment on lines +27 to +28
"MONGODB_ATLAS_PUBLIC_KEY",
"MCLI_PUBLIC_API_KEY",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pick first non empty

Description: "MongoDB Atlas Programmatic Private Key",
Sensitive: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] any reason we were not setting this? can't find any docs that says not to do it when is a configuration

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I'm not aware of any blocker either but @abner-dou @coderGo93 or @thetonymaster - any issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye, yes for private key should be a sensitive, I wasn't aware of that part probably it was there that from the beginning

PrivateKey: d.Get("private_key").(string),
}

if baseURL, ok := d.GetOk("base_url"); ok {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to if here we already if to not use it if empty

@@ -302,14 +315,3 @@ func HashCodeString(s string) int {
// v == MinInt
return 0
}

// HashCodeStrings hashes a list of strings to a unique hashcode.
func HashCodeStrings(hashStrings []string) string {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deadcode

@@ -8,11 +8,13 @@ description: |-

# MongoDB Atlas Provider

The MongoDB Atlas provider is used to interact with the resources supported by [MongoDB Atlas](https://www.mongodb.com/cloud/atlas). The provider needs to be configured with the proper credentials before it can be used.
You can use the MongoDB Atlas provider to interact with the resources supported by [MongoDB Atlas](https://www.mongodb.com/cloud/atlas).
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoiding passive voice


### Environment variables
### Environment Variables
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

env first, static later, lazy readers will stop after first example and may skip the warning is not secure

Copy link
Collaborator

@themantissa themantissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nice add as we do have users with both. Also appreciate the other improvements.
LGTM!

Description: "MongoDB Atlas Programmatic Private Key",
Sensitive: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I'm not aware of any blocker either but @abner-dou @coderGo93 or @thetonymaster - any issue?

Copy link
Contributor

@coderGo93 coderGo93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@themantissa themantissa merged commit e8944c5 into master Aug 27, 2021
@themantissa themantissa deleted the mcli-integration branch August 27, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-testacc To run acceptance tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants